-
Notifications
You must be signed in to change notification settings - Fork 908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Single line if's for 1 stmt block #6031
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sjael thanks for the PR. More work is needed before we could consider moving forward.
First, this new feature should be gated behind an opt-in configuration option. Because of rustfmt's strong formatting guarantees we can't make single-line if
formatting the default.
Second, we'll need many more test cases to show that this is implemented as expected.
Lastly, this option will need to interact well with existing configurations. single_line_if_else_max_width immediately comes to mind, but we probably also want to check how the new configuration interacts with control_brace_style
if result.contains('\n') { | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does result
contain any rewritten attributes? Would the #[allow(unused)]
prevent rustfmt from writing this on a single line?
fn main() {
#[allow(unused)]
if true {
println!("case 1");
}
#[allow(unused)]
// What about if there's a comment here?
if true {
println!("case 2 with comment");
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also don't want to allow single lining if the block contains a comment. I think the block rewriting handles that but just a call out that we'd need a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result
here is the condition string, so it's saying if we haven't gone to a new line due to the condition, proceed with checking if we can go for 1 line with the block. So yes, it contains rewritten attributes if I understand you correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ask because I overlooked this case when adding single_line_let_else_max_width
and then we needed #5902 to correct the issue. I want to make sure that we handle this up front. It would be unfortunate if leading attributes or comments prevented the guard clause from being reformatted.
58fd26a
to
2b9164e
Compare
@ytmimi Test files might need more cases, and names could be changed if needed. Let me know your thoughts on it. |
will take another pass at this when I have some time. In the meantime take a look at the next round of comments I left. |
Made most of the requested changes. I also moved the test file concerning the base functionality of the feature into |
Would be convenient for me if this was merged, is there a particular reason it hasn't been? |
@Astralchroma I appreciate that you're interested in seeing this land, and it hasn't been merged since I haven't had the time to re-review this PR. |
Fixes #5939